Add jacobian_exp implementations for SE(2/3) and SGal(3) - #103

Locally I only got 6 ambiguities:

Number of LieGroups.jl ambiguities: 6
Test Summary: | Pass  Total     Time
LieGroups.jl  | 5463   5463  3m12.4s
     Testing LieGroups tests passed 

Thanks for the PR!
We had 6 ambiguities before, those are fine, see our bound at

Thanks for the transparency on AI, then I will need more time to check the PR more closely – at lest the reference does exist 😄
In order for the @cite to work, though, you have add the corresponding entry also to the https://github.com/JuliaManifolds/LieGroups.jl/blob/main/docs/src/references.bib

edit: Oh I See, on CI it raised to 20; that we should carefully check, why that is the case; It was still 6 on last run of CI here, so I fear it is something in this PR then – and most probably interaction with the Identity https://juliamanifolds.github.io/LieGroups.jl/stable/interface/operations/#LieGroups.Identity

Thanks for the transparency on AI, then I will need more time to check the PR more closely – at lest the reference does exist 😄

I really suck with writing documentation, but did explicitly tell AI what references to use for the docs and know them well, (they were already in use as well).

(they were already in use as well).

Oh sorry! I was a bit quick there, since I only checked that it is not added in this PR, I did (sadly) not check whether it was already in there.

Yeah I am not a fan of AI, but since I get told at least 3 times a day by now I should do everything with AI and spend all my personal money (no support by university there) on some mega-token plan – I gave up arguing against AI and am just fine when people are honest by now.
Usually I have to compensate other peoples use of AI and fixing their mistakes then, that is what I am a bit tired of – but also nothing I can change. It is what life is by now probably, we are all slaves of AI.
And here it just means it might take a while until I get around to check the formulae against the source and such. At least I see the docs do include formulae, which is what we aim for in our packages.

Ahh, I know why I only got 6 ambiguities, my local environment was stil on ManifoldsBase v2.4.0:

(LieGroups) pkg> st
Project LieGroups v0.1.12
Status `~/.julia/dev/LieGroups/Project.toml`
  [1cead3c2] Manifolds v0.11.28
⌃ [3362f125] ManifoldsBase v2.4.0
  [94ee1d12] Quaternions v0.7.7
  [90137ffa] StaticArrays v1.9.18
  [37e2e46d] LinearAlgebra v1.12.0
  [9a3f8284] Random v1.11.0
Info Packages marked with ⌃ have new versions available and may be upgradable.

If I update to ManifoldsBase v2.5.0 I also get the same failure:

Ambiguities: Test Failed at /home/johan/.julia/dev/LieGroups/test/test_aqua.jl:28
  Expression: length(ms) <= LG_LIMIT
   Evaluated: 10 <= 6

The maths part should be fine, that was mostly me, but I did make some mistakes that was picked up. You will see the most complicated is for SGal and i referenced the equations from https://arxiv.org/pdf/2312.07555v11 directly.
I also checked it using monte carlo against the truncated series of the jacobian (test/groups/jacobian_exp_series_reference.jl) and spot checks are included in the tests.

Edit: Objective look would be very welcome as I tested it against my own understanding, so not very objective.

Oh! I remembered that we print the ambiguities, see https://github.com/JuliaManifolds/LieGroups.jl/actions/runs/29268851175/job/86881148111#step:6:316

And my guess was not wrong – it's always the identity – just that this one is on us, it's the new + and - ambiguities with the new ZeroVector type, so its not your fault and you can keep the ambiguity on 10 for now. I hope to remember an issue for that tomorrow then.

I think it is complaining about methods like `+(::Identity{AdditionGroupOperation}, Y::ManifoldsBase.ZeroVector) which is not valid operations. Should we ignore it or maybe throw an ArgumentError?

edit: ok I'll keep on 10 for now.

Actually it is valid:
Identity{Addition} is a valid point on a Lie algebra (not only on the Lie group with group op +) and indeed that identity is the same as the zero vector (because that is the identity in a vector space).

We can really just define those 4 functions to return the identity argument of the two and we are fine.

Indeed both types have exactly the same reason: we want to avoid allocating zeros.

You are correct, I didn't think that far. I'll add those 4 methods tomorrow.

Like this:

Base.:+(g::Identity{AdditionGroupOperation}, ::ManifoldsBase.ZeroVector) = g
Base.:+(::ManifoldsBase.ZeroVector, g::Identity{AdditionGroupOperation}) = g
Base.:-(g::Identity{AdditionGroupOperation}, ::ManifoldsBase.ZeroVector) = g
Base.:-(::ManifoldsBase.ZeroVector, g::Identity{AdditionGroupOperation}) = g

Yes. Exactly that. Perfect :)
And sure, it is two languages meeting basically, the zero vector was even inspired by the identity, so I think its just fair to resolve that here.

Thanks for adding them even here (and fixing our problem there) – maybe even add a short note to the changelog just for completeness :)

I'm a bit confused here. Is this the Jacobian of the group exponential or special cases of jacobian_exp_argument or jacobian_exp_basepoint? You do specify the g but don't specify with respect to what the Jacobian is calculated.

Oh I was confused by that as well, but wanted to first check more closely whether that is on me ;)

To provide detail here: the base point one is the differential with respect to g (X fixed) and the arg one is the one with respect to X (g fixed).
Since the Lie group exponential has a fixed g=e (the identity), the Lie group one only has the argument one – but then no g. Or to even make it more precise, X is then also always from the Lie algebra.

.... Since the Lie group exponential has a fixed g=e (the identity), the Lie group one only has the argument one – but then no g. Or to even make it more precise, X is then also always from the Lie algebra.

I saw this and followed the existing pattern, eg:

I wasn't too sure about the g argument though.
If g is not at the identity will this be equivalent to jacobian_exp_argument?

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 100.00%. Comparing base (1a25650) to head (5c3159b).

@@            Coverage Diff             @@
##              main      #103    +/-   ##
==========================================
  Coverage   100.00%   100.00%            
==========================================
  Files           36        36            
  Lines         2899      3066   +167     
==========================================
+ Hits          2899      3066   +167     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

I see, I forgot that already exists. I've checked the discussion in #65 and I have no clue why the _argument suffix was dropped but the point argument was not. Either both should be dropped or both kept. Maybe this is a good time to make this straight. @kellertuer , is there a reason behind this convention or was that an oversight? If it's the second case, do you prefer adding _argument or dropping p/g?

Hm, I mean this #65 (comment) definitely says that we only have exp_arg here, so there is no need to do the base one – and to then pass on the the manifold (arg) jac exp?.

I think what we could do, is the same as the default described herre
https://juliamanifolds.github.io/LieGroups.jl/stable/interface/group/#Base.exp-Tuple{AbstractLieGroup,%20Any,%20Any}
compared to here
https://juliamanifolds.github.io/LieGroups.jl/stable/interface/group/#Base.exp-Tuple{AbstractLieGroup,%20Any}

and do kind of both – but one for each.
For the first – with g – that defaults to the second anyways, we introduce the arg one – pushing forward the second case – allowing for a nice consistency with manifolds/tangent-spacey X
For the second case we can introduce the drop-g/p case.

I am just not sure which should be the default to implement, but I think the second one is probably nicer to implement and the first push-forward-defaults.

I hope this is – given the early evening time – not written in too confusing words.

Yes, that comment explains that it's essentially jacobian_exp_argument, and it even comments that g may not be necessary, so I think it was just an oversight that g wasn't removed in that PR.

and do kind of both – but one for each.
For the first – with g – that defaults to the second anyways, we introduce the arg one – pushing forward the second case – allowing for a nice consistency with manifolds/tangent-spacey X
For the second case we can introduce the drop-g/p case.

That makes sense to me.

I am just not sure which should be the default to implement, but I think the second one is probably nicer to implement and the first push-forward-defaults.

The "no g" case as the default to implement sounds good. So basically all this PR needs to do is drop g and in the next PR we can fix the other groups I think.

The "no g" case as the default to implement sounds good. So basically all this PR needs to do is drop g and in the next PR we can fix the other groups I think.

The interface and tests are also affected and it looks like only one group was implemented (SO2/3). So it looks easier to drop and deprecate the g parameter in this branch. I'm busy removing it for you to look at.

Hi @kellertuer, so this is little sucky and sorry to push on it, but...

TLDR; I'm a bit upset at what looks like a double standard, and I think we have contributed enough to not be so buried. I would like to see "[J. Terblanche and] NavAbility Contributors et al" at the same level as any other contributors, with clear distinction from "Maintainers" per your comment.

Just so you are aware, we have contributed easily $30k+ specifically to LieGroups.jl's benefit -- i.e. made possible Johan's 11 PRs thus far (which are original or derive from earlier work by others). I would have thought you'd want to expand the number of maintainers. Please notice that I'm not trying to get my name on your this repo nor on Manifolds.jl / Manopt.jl / etc.

Guess, I'd feel better if you

• Apply one standard, i.e. remove non-active maintainers from the "authors" list in Project.toml. The tiers are confusing, creator/maintainer/non-active-maintainer/member/contributor. This is one of the reasons why we use "NavAbility Contributors" everywhere.
• Update the README with clear indication of live maintainers vs. contributors, e.g. remove the excess contributors at the bottom of README.md and point with active links to
  • See contributors at zenodo
  • See active active maintainers
• Since your name is already on the JuliaCon proceedings, I'd also suggest you create a prominent link to the paper, alongside other important literature, without repeating a few select authors (distracting from the suggested links).

Yeah, bit sucky but I wouldn't push if I didn't think it is a fair request. Also, I will not push the topic again, but please know that this does matter.

Current screen capture

Sorry, I do not understand what you are writing

we have contributed easily $30k+ specifically to LieGroups.jl's benefit

I have not yet seen a single cent in what so ever from my open source contributions.
If you mean, you invested that money intro developing your own open source packages. Nice, that you got that amount of money.

I do all this somewhere in my free time and evenings and I am currently getting more and more backlash – my packages are no good – I should spent more time on grants / AI / AI / AI / producing less bugs / produce better packages / ...

Apply one standard,

There is exactly one standard. Each contributor listed was a long time contributor to either this package directly, or for Seth and Olivier, the main contributors to the code base that was “extracted” from Manifolds.jl. This is exactly one standard that I already explained. That is exactly one consistent standard throughout all JuliaManifolds packages.

Update the README with clear indication of live maintainers vs. contributors,

That is already the case: The proceeding paper accompanying last JuliaCons talk lists the two active maintainers. The Zenodo former and current contributors – the Project.toml lists all former and current maintainers..

Also there is no excess – all references are collapsed by default, your screenshot is very misleading.

I'd also suggest you create a prominent link to the paper, alongside other important literature, without repeating a few select authors (distracting from the suggested links).

This I simply do not understand. Literature is listed on the References page in the docs? I am unsure what you refer to here.

Note that the previous discussion on your proposal of a change was not even closed by me but by Johannes. There I explained how we handle naming contributors, what distinguishes them from maintainers. I have nothing to add to that explanation.

But I can repeat that here again: Johannes is already listed on the about page: https://juliamanifolds.github.io/LieGroups.jl/stable/about/ – he can join the metadata. That's it. The company will not be added extra.

The money note I absolutely do not get. What are you trying to indicate there? That you can buy something? That you are cool?
As I wrote I do this in my free time – LieGroups.jl especially. Last year the JuliaCon people were kind enough to waive the conference fee for me so I could afford to attend. Otherwise (see above as well) I do not have much grants – and can for example not afford to attend this years JuliaCon.
LieGroups.jl is to zero extend related to my research, so it is totally and fully a hobby project – and I put several months into this to get the package started.
From the attendence/talk at JuliaCon last year I at leat got the small proceedings paper.

But you are not alone, I get more and more complaints recently: Manopt.jl is not performant enough – that could all be done much much better, things like that.
From a nice thing I do in my free time, I by now spent quite some time answering posts like yours that complain.
Maybe I should leave open source then if no one like what or how I do things? Is that what you plan to achieve? Especially this year, I got a few times to the point considering that.
(And sure, once there is active maintainer left – you can take over the package and maybe even rename it to NavAbilityLieGroups.jl or so, if that is what you aim to achieve polemically phrased.)

So, TLDR: I explained the rules and offered what we can do and what the standard is (there is no double standard – again). The company name is a no go for me also in the metadata.
And I do – to zero extend – understand neither your tone nor your intentions.

Let me know what you think – if you like, also about my thoughts whether I should continue the development or your intentions are to stop me from contributing to FOSS.

Hi @kellertuer — for my part, I think the recognition here is already good — my name and affiliation (WhereWhen.ai Technologies Inc., operating as NavAbility) are listed as a contributor in .zenodo.json, which I think covers what matters to me. I don't have a strong opinion on the maintainer/contributor tiers beyond that, that's your call to make as the person who's carried this project.

I've genuinely learned a lot working through this and previous PRs with you and think these have been a team effort, with a lot of help along the way from you and Mateusz.

Happy to keep going on the remaining review comments whenever you have time.

I was now even confused for a second – since this PR does not change the Zenodo metadata – but you already are in there. And sure therein, the affiliation is totally up to you.

For this PR, the only thing I noticed now in the files I have not yet marked as read: In the extension we now have doc strings. This also would lead to the point they can only “be read” once the extension is loaded.
Still the string itself does not mention the points being something “special” (only available in the extension). Might that confuse users?

Besides that, we should only remember to set the date in the news before merging the PR.

So again concerning maintaintership:

• Yueh-Hua Tu is listed there, since he started this package of this name originally, so he was maintainer here – so he of course stays in there
• Seth started the implementation the current form is based on – just back in Manifolds.jl – but Seth has at some point stepped down as maintainer, when he wanted to reduce his workload on FOSS
• Olivier contributed a lot to Seths implementation, so I asked whether he wants to join that list.
• Mateusz and myself are the two active ones.

From a maintainer I would expect to work on issues opened by users, actively fix bugs and such – and yes with my other obligations and this package not seeming to be used much (or at least less than the others), I did not find the time to work on those.
A maintainer should at least try to find time to work on these, react to new users discussions / issues / ...
For me that both requires a good understanding of both the code base and the theory (so here the math) behind.

A company can not do that per se. So a company as a maintainer is a no go for me (unless all maintainers are from that company maybe). Whether you feel you want to step up to those obligations mentioned – I do not know?

For this PR, the only thing I noticed now in the files I have not yet marked as read: In the extension we now have doc strings. This also would lead to the point they can only “be read” once the extension is loaded.
Still the string itself does not mention the points being something “special” (only available in the extension). Might that confuse users?

Good point on the doc strings. Right now the whole SGal group implementation depends on the RecursiveArrayTools extension (via ArrayPartition) — there's no pure matrix-form implementation yet. So it's less "these specific functions need the extension" and more "SGal itself only works once the extension is loaded."

The SGal group constructor does have a "The ArrayPartition (default) implementation requires RecursiveArrayTools.jl to be loaded. The matrix representation is not implemented yet." line.

I looked at ProductLieGroup as that also only works with the RAT extension, that throws a clear error when you try to use it: "This error could be resolved by importing RecursiveArrayTools.jl.". The ProductLieGroup doc strings doesn't mention RAT.

The "This error could be resolved by importing RecursiveArrayTools.jl." way might be the clearest, but I could not find that yet.

Besides that, we should only remember to set the date in the news before merging the PR.

I saw you normally set the date and bump the version just before release in a separate PR.
PowerLieGroup is also missing jacobian_exp, it should not be hard to do and I can add it here or in a new PR if we want to get this one merged.

Luckily Maintainers are also Magicians. ;) (Joke aside, it helps a bit when maintainers have a reasonable overview on the code base)

There is two ways to provide a good error (besides adding the note to the doc string).

A very clean method is adding error hints like

https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/42478fd08aa0e6f5520091b324af057d4e846290/src/ManifoldsBase.jl#L1285-L1301

and I was surprised we have nearly none for RAT. There we used another trick, probably from before the error hints existed

https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/42478fd08aa0e6f5520091b324af057d4e846290/src/ManifoldsBase.jl#L197-L203

we check whether one allocates on a product manifold hit that default case there. Before you reach this array case, you come from allocate_on. loading RAT this is overwritten at

https://github.com/JuliaManifolds/ManifoldsBase.jl/blob/42478fd08aa0e6f5520091b324af057d4e846290/ext/ManifoldsBaseRecursiveArrayToolsExt/ProductManifoldRecursiveArrayToolsExt.jl#L33-L35

This is where you see that message from :) or do not see it after loading

I personally like the hint better since it keeps the original error message.

PowerLieGroup is also missing jacobian_exp,

since we twiddle with that function here, we can also do that here, no need to do a separate PR I think.